Add Exception Type SubOrchestrationFailedException
#398
Conversation
| coreEx.GetType().ToString(), | ||
| coreEx.Message, | ||
| coreEx.StackTrace, | ||
| FromCoreFailureDetailsRecursive(coreEx.FailureDetails?.InnerFailure) ?? FromExceptionRecursive(coreEx.InnerException)); |
There was a problem hiding this comment.
Should this be
FromCoreFailureDetailsRecursive(coreEx.FailureDetails) ?? FromExceptionRecursive(coreEx.InnerException))
To prevent loss of the outer FailureDetails?
| // Outer failure represents the orchestration failure | ||
| Assert.NotNull(ex.FailureDetails); | ||
| Assert.True(ex.FailureDetails.IsCausedBy<TaskFailedException>()); | ||
| Assert.Contains("ThrowException", ex.FailureDetails.ErrorMessage); |
There was a problem hiding this comment.
This change worries me - our tests were checking that the activity name was present in the exception message, which I think is important. Even if this change means that this information is in some InnerFailure, can we still make sure that this information is present somewhere in the exception details?
| Assert.NotNull(ex.FailureDetails); | ||
| Assert.True(ex.FailureDetails.IsCausedBy<TaskFailedException>()); | ||
| Assert.Contains("ThrowException", ex.FailureDetails.ErrorMessage); | ||
| Assert.True(ex.FailureDetails.IsCausedBy<CoreSubOrchestrationFailedException>()); |
There was a problem hiding this comment.
We don't want to expose any DurableTask.Core types in the .NET Isolated model. Also, this is a breaking change so I think we need to rethink this.
There was a problem hiding this comment.
yeah what I’m mainly worried about with this change is that it looks like we don’t want to expose any core exception types here, though I’m not sure why.
I’m totally fine with closing this. I made this change mainly because it’s what the issue/customer was asking for and with this change it seemed more in line with our in-proc model. But it seems like we just want to show that the sub-orchestrator failed in the error message, not the actual error type, is that correct?
jviau
left a comment
jviau
left a comment
There was a problem hiding this comment.
We shouldn't expose any DurableTask.Core types to the user in this repo, we want to keep that purely an implementation detail. Please derive a new exception type in this repo and construct that from the SubOrchestrationFailedException instead
nytian
changed the title
SubOrchestrationFailedException
|
After further discussing and consideration, this PR should be closed. The main reason is that our current approach—throwing |
This PR partially solves issue Azure/azure-functions-durable-extension#2689
This PR adds a new exception type
Microsoft.DurableTask.SubOrchestrationFailedException. Previously,SubOrchestrationFailedExceptionwas included with theTaskFailedException, and it's now handled separately with this new type.For example, if cx want custom handling logic specifically for suborchestration failures, they should write a catch block for SubOrchestrationFailedException instead of the broader TaskFailedException.